fix(consensus): hold delegated EOAs to their key in delegateStake - #165
Conversation
`delegateStake` verified the validator's EIP-712 approval with `SignatureCheckerLib.isValidSignatureNowCalldata`, which branches on `extcodesize` first. An EIP-7702 delegated EOA carries 23 bytes of `0xef0100 || implementation`, so the branch is not taken, `ecrecover` is never reached, and the check becomes an ERC-1271 staticcall against whatever wallet program the account currently points at. That substitutes the wrong authority in both directions. A validator on a 7702 wallet without an ERC-1271 handler cannot be onboarded at all: its valid key signature is ignored and the call reverts `NotValidator` with nothing to explain why. In the other direction, a permissive handler will approve the digest, which `tn_delegationDigest` publishes, letting anyone who can reach it bind an arbitrary delegator. `_getRecipient` routes every subsequent `claimStakeRewards` and `unstake` to that delegator, so the loss is the validator's whole stake and reward stream. Guardrails narrow who is exposed - the caller still needs the validator's BLS key, the full stake in `msg.value`, and `Undefined` status - but not the severity for anyone in that set. Route on the account kind instead of on code size. A delegation designator is matched by its prefix, which EIP-3541 makes unambiguous, and delegated EOAs recover through `ECDSA.tryRecoverCalldata` exactly as undelegated ones do. Only genuine contract accounts reach ERC-1271. An operator who no longer holds the underlying key still has `stake`, which authorizes off `msg.sender`, and the governance path, which skips the signature entirely. Closes #163 Alongside it, pin the ejection paths against the same class of change. Governance screens a prospective validator before minting, but the holder can attach account code afterwards, so screening is not durable. `burn` and the slash-to-zero branch of `applySlashes` are already push-free: both zero the balance ledger and consolidate the stake on Issuance before closing the token out, so `_unstake` returns at its zero-balance guard without paying anyone. That property was incidental rather than structural, and it was obscured by `_consensusBurn` calling a function named `_unstake` with a recipient it never used. Split `_burnConsensusNFT` out so the confiscating path closes the token directly, and cover both ejection paths with hostile validator and delegator account code, including the escrow case, which is the one balance an ejection owes back and which already credits `claimRefund` rather than pushing. Closes #164
…variant
Audit follow-ups on the 7702 work, no source change.
Three cases the first pass left uncovered on the delegated-EOA route: a
65-byte signature with a `v` outside {27, 28}, an empty signature, and a
differential check that the designator branch accepts exactly the signature
set the plain EOA branch accepts. The last one probes the known divergence
candidate, high-s malleability, which neither solady route screens. It is
inert here since the digest binds the delegator and a nonce and a successful
call leaves `Undefined` status behind, but the two routes must agree or the
designator branch becomes its own authorization surface. All three fail
against the pre-fix source.
`invariants.md` gains the two properties a reviewer should be able to check
the code against: which authority `delegateStake` accepts for each kind of
account, and that neither ejection path makes a recipient-facing external
call.
Audit pass before reviewRe-reviewed the change adversarially. No defects found; three gaps closed in Verified
Gaps closedThree cases were uncovered on the delegated-EOA route, now tested and each failing against the pre-fix source:
The parity test probes the one real divergence candidate. Neither solady route screens high-s, so
Behaviour change worth a reviewer's eyeAn operator who moved to a 7702 smart account and no longer controls the underlying key can no longer be onboarded through CoordinationBytecode changed, so the node's genesis allocation needs the regenerated Test status
|
Closes #163, closes #164.
#163 -
delegateStakesilently switches from ecrecover to ERC-1271Confirmed, both halves, with tests that fail on
master.SignatureCheckerLib.isValidSignatureNowCalldatabranches onextcodesizebefore anything else. A 7702-delegated EOA carries 23 bytes of0xef0100 || implementation, so the branch is not taken andecrecoveris never reached. The check becomes an ERC-1271 staticcall against whatever the EOA currently delegates to.test_delegateStake_delegatedEOA_acceptsKeySignaturerevertsNotValidatoronmastereven though the validator signed the digest with its own key. Any 7702 wallet without a matching ERC-1271 handler is unonboardable, with nothing in the revert to say why.testRevert_delegateStake_permissiveDelegateCannotAuthorizesucceeds onmasterwith the four-byte signature0xdeadbeef. The digest is public viatn_delegationDigest, so a permissive handler is enough to bind an arbitrary delegator, and_getRecipientsends every laterclaimStakeRewardsandunstakethere.Fix
Route on what kind of account the validator actually is rather than on code size:
The designator is matched by its
0xef0100prefix, not merely its length. EIP-3541 forbids deployed code from starting with0xEF, so the prefix is unambiguous, and a real 23-byte contract still routes to ERC-1271 (testRevert_delegateStake_twentyThreeByteContractIsNotADesignator).Delegated EOAs are now held to their secp256k1 key exactly as undelegated ones are; only genuine contract accounts reach ERC-1271, and
test_delegateStake_contractValidator_usesERC1271pins that we did not break them. An operator who has moved to a smart account and no longer holds the key still has two routes in:stake, which authorizes offmsg.sender, and the governance path, which skips the signature check entirely.#164 - 7702 dissolves EOA screening
The premise holds - screening at mint time is not durable, a holder can attach a reverting delegation afterwards - but the described exploit chain does not reproduce.
burnand the slash-to-zero branch ofapplySlashesboth succeed today against a validator whose account rejects every incoming call._consensusBurnzeroesbalances[validatorAddress]and consolidates the stake on Issuance before reaching_unstake, so_unstakereturns at its zero-balance guard and never callsdistributeStakeReward. No value is pushed to the validator, and there is no burn payout to convert to a pull-based credit.What was true is that the property was incidental rather than structural, and actively obscured:
_consensusBurncalled a function named_unstake, passing arecipientit computed and never used. So rather than a no-op fix, this PR makes the property explicit and pins it:_burnConsensusNFTis split out of_unstake(token burn, supply guard, delegation clear - no value movement) and the confiscating path calls it directly. Behavior is unchanged; the misleading payout call is gone.claimRefundrather than pushing. The slash test also concludes the following epoch, since that path runs inside a system call where a revert stalls the closing block rather than merely inconveniencing governance.These four pass on
mastertoo. That is the point: they document a property that currently holds by accident and would otherwise be easy to refactor away.nonReentrantis deliberately not added toburn. It isonlyOwnerand, after this change, provably makes no call to an untrusted address.Testing
test/consensus/ConsensusRegistryEIP7702Test.t.solmaster: the four delegateStake silently switches from ecrecover to ERC-1271 #163 tests fail (twoNotValidator, two "did not revert as expected"), the four 7702 dissolves EOA screening as a durable mitigation #164 tests passfastandciprofiles: 324 passed, 0 failed, 2 skippedAlso included
artifacts/ConsensusRegistry.jsonregenerated, since the source changed and the committed artifact is consumed downstream. Docs updated: thedelegateStakenatspec now states which authority is competent to sign, anddesign.mdgains bullets for delegation authorization under account abstraction and for the push-free ejection property.